[3.0] Changes WebApp to use Express under the hood #12442

Grubba27 · 2023-01-13T19:50:35Z

Sorry about the number of files and changes

This PR focuses on changing connect framework for the Express framework.

Why is this happening?

The way connect handles async handlers is kind of awkward because it was made at a different time than ours, while we were removing fibers we decided to do another upgrade in the MeteorJS core. Give a more modern-ish look for core functionality.

To make async code work properly you would need to do something like this in your codebase:

 app.use(async function middleware1(req, res, next) {
    // middleware 1
   // if error occurs will rang
    const getFoo = await FooCol.find({}).fetchAsync();
    next();
  });

 //this is fine
 app.use(async function middleware2(req, res, next) {
    try{
      const getFoo = await FooCol.find({}).fetchAsync();
      next();
    } catch (e){next(e)}
  });

We could have a function like @radekmie has shown me:

function wrap(fn) {
  return (request, response, next) => {
    Promise.resolve().then(fn).then(() => next(), next);
  };
}

and could be used like this to have the same functionality that we have in Express:

WebApp.connectHandlers.use(wrap(async (request, response) => {
  // ...
}));

Now with the new API the code above can be replaced with the following:

WebApp.expressHandlers.use(async (request, response) => {
  // ...
});

Does not seems much, but this will help us a lot in the core, and the community in the long term, as we can benefit a lot more from having 100% compatibility with the express plugin ecosystem.

But express was built on top of connect why the change?

From express docs:

Express 4 no longer depends on Connect, and removes all built-in middleware from its core, except for the express.static function. This means that Express is now an independent routing and middleware web framework

This is an overall improvement as we can do it all the same way as before but now our webapp package is a little bit more complete.

What changes for me?

this was extracted from the history.md

WebApp.connectHandlers.use(middleware) is now WebApp.expressHandlers.use(middleware)
WebApp.rawConnectHandlers.use(middleware) is now WebApp.rawExpressHandlers.use(middleware)
WebApp.connectApp is now WebApp.expressApp

If you have an edge case that may break because of compatibility issues here is express 4 migration guide

Running against devel tests:

This closes #12447

changes connect by express

filipenevola

It seems very interesting.

I would like to see this PR against current devel/master just to make sure everything is still working as expected.

Later we could apply it on top of 3.0 as I think this would be the right place to merge it.

What do you think @Grubba27 ?

Grubba27 · 2023-01-14T00:02:11Z

Sounds great!
I will try do this as soon as possible!
I did tried running all tests, all but 1 ran smoothly, I did not waited it finish but I think it will be okay.

radekmie

My only concern about this change are the defaults of connect and express. For example, I think we definitely should opt out of X-Powered-By:

app.disable('x-powered-by');

And there are other things like that, including ETags. It may affect the performance and probably security significantly.

packages/webapp/webapp.d.ts

Co-authored-by: Radosław Miernik <radekmie@gmail.com>

Grubba27 · 2023-01-16T12:36:22Z

And there are other things like that, including ETags. It may affect the performance and probably security significantly.

Can you enumerate a little bit more? I will make the adjustments with app.disable('x-powered-by'); and app.set('etag', false);
cc: @radekmie

Grubba27 · 2023-01-16T13:25:43Z

Running against devel/ without fibers
cc: @filipenevola

zodern · 2023-01-16T15:11:45Z

Replacing connect with something that has a router will be very nice.

It seems one of the main reasons for doing this now is express handles promise rejections better. This is coming in express 5, which is still in beta. As far as I know, express 4 doesn't handle it any better than connect does. We already modify connect so it supports fibers. We could do the same thing with connect or express so it handles rejected promises.

Since there are so many breaking changes in Meteor 3, I'm wondering if there is a way to switch to express in a way that doesn't add more. It is possible to use express as a middleware for connect. Instead of removing connect completely, we could keep it for now, and add WebApp.expressHandlers which runs after WebApp.rawConnectHandlers and maybe before WebApp.connectHandlers. Existing code could work as is until projects are ready to migrate, and new code can use express. Later connect could be deprecated and removed.

Grubba27 · 2023-01-16T17:54:15Z

IMHO this:

We could do the same thing with connect or express, so it handles rejected promises.

It would be the best, but at the same time, I think that if we have the chance to move into something that is more well-known, it would be good for MeteorJS adoption.

Regarding the amount of changes that we will have on this 3.0, it seems overwhelming. At the same time, it would be hard to maintain both connect and express. Let's take this chance of having a breaking change and move a few pieces because if we cannot change now, when can we?

radekmie · 2023-01-16T20:23:06Z

And there are other things like that, including ETags. It may affect the performance and probably security significantly.

Can you enumerate a little bit more? I will make the adjustments with app.disable('x-powered-by'); and app.set('etag', false);

@Grubba27 First of all, I think it has to be done for every express instance, not only the top-level one. Secondly, I'm not sure there's anything else, but the best and easiest would be to run the app with and without this change, save curl -v localhost:3000 result to a file and compare the headers and the rest of the output for a few endpoints (/, some dynamic import, a static file).

Grubba27 · 2023-01-16T22:17:05Z

With express without disabling etag and x-powered-by:

With connect:

With express disabling etag and x-powered-by

cc: @radekmie

rj-david · 2023-01-18T09:30:34Z

What changes for me?

this was extracted from the history.md

WebApp.connectHandlers.use(middleware) is now WebApp.expressHandlers.use(middleware)

WebApp.rawConnectHandlers.use(middleware) is now WebApp.rawExpressHandlers.use(middleware)

WebApp.connectApp is now WebApp.expressApp

@Grubba27, am I correct with the following migration paths:

Custom handlers, just update the object to WebApp.express*
Npm middlewares, same as number 1
Meteor packages (e.g. restivus) - fork the package and do number 1

Did I miss anything?

packages/webapp/webapp.d.ts

packages/webapp/webapp_server.js

Grubba27 · 2023-01-18T20:26:48Z

Hey, @rj-david, I would say that it Is a good practice just take a look at express 4 migration just in case you use a very specific connect feature that was not ported to express, I will update the docs & PR description to reflect this

# Conflicts: # docs/history.md

henriquealbert

Great work as always @Grubba27 🎉

StorytellerCZ · 2023-01-19T08:17:12Z

Well #12410 happened much earlier than I expected. 😂

Grubba27 added 18 commits January 13, 2023 12:32

chore: added express instead of connect

be725b8

docs: added history about changes in webapp

d427749

changes connect by express

docs: changes rawConnectHandlers by rawExpressHandlers

08e91e7

docs: changes connect by express in security.md

91af432

docs: changes connect docs by express docs

2fe9d36

chore: changes appcache-server.js connect by express

3609a65

chore: update browser-policy-common to use rawExpressHandlers

dd223a8

chore: update test_responder to use express

8ec1f6b

chore: spiderable_server is now using express

44547aa

chore: updated oauth_server to use expressHandler

ad76faf

chore: added packages in lock file

80a274b

chore: removed connect.js file

d233c04

chore: remove connect from webapp

929da0d

docs: updated webapp readme

f2f2c4e

types: updated webapp types

62066ac

chore: update apollo skeleton with new express app

7cd854b

feat: added express to webserver

560c1b3

tests: adjusted test to match express npm modules

ae3e31f

Grubba27 requested review from radekmie, zodern and denihs January 13, 2023 19:50

github-actions bot added Project:Docs Project:Guide Project:Tool Project:Utilities:HTTP Project:Webapp Project:Webapp:Browser Policy labels Jan 13, 2023

github-actions bot temporarily deployed to pull request January 13, 2023 19:51 Inactive

NicolasLopes7 approved these changes Jan 13, 2023

View reviewed changes

filipenevola reviewed Jan 13, 2023

View reviewed changes

radekmie reviewed Jan 14, 2023

View reviewed changes

packages/webapp/webapp.d.ts Outdated Show resolved Hide resolved

Update packages/webapp/webapp.d.ts

2b91f52

Co-authored-by: Radosław Miernik <radekmie@gmail.com>

github-actions bot temporarily deployed to pull request January 16, 2023 12:33 Inactive

chore: addressed security comments

8296afc

github-actions bot temporarily deployed to pull request January 16, 2023 13:03 Inactive

Grubba27 mentioned this pull request Jan 16, 2023

Add express as engine #12447

Closed

denihs approved these changes Jan 16, 2023

View reviewed changes

chore: add raw handlers

e4a3fb5

github-actions bot temporarily deployed to pull request January 16, 2023 22:45 Inactive

radekmie reviewed Jan 18, 2023

View reviewed changes

packages/webapp/webapp.d.ts Outdated Show resolved Hide resolved

packages/webapp/webapp_server.js Outdated Show resolved Hide resolved

Grubba27 added 2 commits January 18, 2023 17:19

types: adressed wrong typings

829bf9f

chore: created abstraction on top of express creation

1b921ec

radekmie approved these changes Jan 18, 2023

View reviewed changes

Grubba27 added 2 commits January 18, 2023 17:40

updated last express method

1d3cfca

Merge branch 'release-3.0' into release-3.0-tests-webapp

e5e23fb

# Conflicts: # docs/history.md

github-actions bot temporarily deployed to pull request January 18, 2023 20:46 Inactive

henriquealbert approved these changes Jan 18, 2023

View reviewed changes

StorytellerCZ approved these changes Jan 19, 2023

View reviewed changes

Grubba27 merged commit bb892de into release-3.0 Jan 19, 2023

StorytellerCZ mentioned this pull request Jan 19, 2023

Remove Fibers dependency Meteor-Community-Packages/picker#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3.0] Changes WebApp to use Express under the hood #12442

[3.0] Changes WebApp to use Express under the hood #12442

Grubba27 commented Jan 13, 2023 •

edited

filipenevola left a comment

Grubba27 commented Jan 14, 2023

radekmie left a comment

Grubba27 commented Jan 16, 2023

Grubba27 commented Jan 16, 2023 •

edited

zodern commented Jan 16, 2023

Grubba27 commented Jan 16, 2023

radekmie commented Jan 16, 2023

Grubba27 commented Jan 16, 2023 •

edited

rj-david commented Jan 18, 2023

What changes for me?

Grubba27 commented Jan 18, 2023

henriquealbert left a comment •

edited

StorytellerCZ commented Jan 19, 2023

[3.0] Changes WebApp to use Express under the hood #12442

[3.0] Changes WebApp to use Express under the hood #12442

Conversation

Grubba27 commented Jan 13, 2023 • edited

Why is this happening?

But express was built on top of connect why the change?

What changes for me?

Running against devel tests:

filipenevola left a comment

Choose a reason for hiding this comment

Grubba27 commented Jan 14, 2023

radekmie left a comment

Choose a reason for hiding this comment

Grubba27 commented Jan 16, 2023

Grubba27 commented Jan 16, 2023 • edited

zodern commented Jan 16, 2023

Grubba27 commented Jan 16, 2023

radekmie commented Jan 16, 2023

Grubba27 commented Jan 16, 2023 • edited

rj-david commented Jan 18, 2023

What changes for me?

Grubba27 commented Jan 18, 2023

henriquealbert left a comment • edited

Choose a reason for hiding this comment

StorytellerCZ commented Jan 19, 2023

Grubba27 commented Jan 13, 2023 •

edited

Grubba27 commented Jan 16, 2023 •

edited

Grubba27 commented Jan 16, 2023 •

edited

henriquealbert left a comment •

edited